Reduce russh write-path copies with direct Bytes sends - #695
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes russh’s outbound write path by enabling a direct Bytes -> PacketWriter -> encrypted output fast path when ordering constraints allow, reducing intermediate plaintext staging/copies while preserving packet ordering and encryption behavior.
Changes:
- Introduces
PacketWriter::write_packetin-place encryption (plus supporting APIs) and routes more writes throughPacketWriterdirectly when safe. - Adds direct/with-writer channel data flush paths (including replay after window adjust / rekey completion) and new
Channel::*_bytesAPIs for copy-freeBytescallers. - Hardens handling of peer-provided zero
recipient_maximum_packet_sizeto return an error (instead of panicking / misbehaving), and expands test coverage (incl. optional zlib).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
russh/tests/test_max_channel_packet_size.rs |
Extends max-channel-packet-size regression test to cover optional zlib compression. |
russh/src/sshbuffer.rs |
Adds in-place PacketWriter::write_packet, retained Bytes packet APIs, and related unit tests. |
russh/src/session.rs |
Adds direct channel-data write/flush paths via PacketWriter and switches retained KEXINIT storage to Bytes. |
russh/src/server/session.rs |
Routes server session channel writes through the new Encrypted::*_with_writer fast path. |
russh/src/server/mod.rs |
Adjusts rekey flow to apply new keys before replaying pending channel data through PacketWriter. |
russh/src/server/kex.rs |
Updates packet emission to use write_packet; stores incoming KEXINIT as Bytes. |
russh/src/server/encrypted.rs |
Replays pending channel data through PacketWriter on window adjust to avoid staging copies. |
russh/src/negotiation.rs |
Emits KEXINIT via packet_bytes and returns retained KEXINIT as Bytes. |
russh/src/lib_inner.rs |
Removes an unused debug import (minor cleanup). |
russh/src/kex/hybrid_mlkem.rs |
Updates tests to the new Exchange { *_kex_init: Bytes } type. |
russh/src/compression.rs |
Adds Compress::compress_into to write compressed payloads directly into an output buffer. |
russh/src/client/session.rs |
Routes client session channel writes through the new Encrypted::*_with_writer fast path. |
russh/src/client/mod.rs |
Adjusts rekey flow to apply new keys before replaying pending channel data through PacketWriter. |
russh/src/client/kex.rs |
Updates packet emission to use write_packet; stores incoming KEXINIT as Bytes. |
russh/src/client/encrypted.rs |
Replays pending channel data through PacketWriter on window adjust; updates one packet emission call site. |
russh/src/cipher/mod.rs |
Introduces SealingKey::finish_packet to support in-place packet finalization/sealing. |
russh/src/channels/mod.rs |
Adds data_bytes / extended_data_bytes APIs to send owned Bytes without copying into the AsyncWrite path, with tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1f3a306 to
0bfd1ac
Compare
|
if desired I can add the benchmark harness, also. just wasn't sure it would be wanted |
|
Thank you so much! It will take me a while to properly review since I'm currently away until the 1st - but from a cursory glance everything looks good 👍 |
|
Yeah, sorry about the size of it. if it helps I could split it into a bunch of smaller PRs for easier examination when you're back. There are a couple ways to slice this particular pie that might make it simpler. |
|
Thanks a ton, great work 👍 |
Summary
This PR reduces write-path copying in
russhby moving more packet assembly directly intoPacketWriterand by sending owned channel payloads asByteswhere ordering allows.For channel data, the new fast path writes
Bytespayloads straight intoPacketWriterinstead of first staging plaintext intoEncrypted.write. That path is used only when the channel is confirmed, no rekey is in progress, there is no older pending data for that channel, and the session write queue is empty. If any of those conditions are not met, behavior falls back to the existing queued path.The same direct-writer path is now used when replaying pending channel data after a window adjustment or after rekey completion, again only when ordering permits. This keeps the existing ordering guarantees while avoiding the extra staging copy on the replay path too.
API additions
This also adds owned-
Byteschannel send helpers:Channel::data_bytesChannel::extended_data_bytesChannelWriteHalf::data_bytesChannelWriteHalf::extended_data_bytesThese helpers split payloads by writable window and max packet size using
Bytes::slice(...), so callers that already own aBytesbuffer can enqueue channel data without the extra copy inherent in theAsyncWrite<&[u8]>path.Packet writer changes
To support the direct path,
PacketWriteris refactored so it can assemble and seal packets directly into its output buffer when compression is disabled, while still keeping a reusable plaintext packet buffer for retained-packet and compressed cases. The compressed path is also tightened so compressed output can be written directly into the final output buffer.Retained KEXINIT-style packets are now kept as
Bytes, and the stored client/server KEXINIT payloads inExchangeuseBytesas well.Hardening and validation
This PR also hardens channel write sizing by returning
Error::Inconsistentwhen a peer advertises a zero max packet size instead of panicking.Coverage was expanded around:
PacketWriterWall-Clock Measurements
Temporary harnesses, not committed. Base is updated
mainatc31cbc9.Numbers below are median-of-5 runs from the same local harness.
Encrypted.writestagingBytes->PacketWriterEncrypted.writestagingBytes->PacketWriterEncrypted.writePacketWriterEncrypted.writePacketWriterAsyncWrite<&[u8]>copy pathBytesAPIAsyncWrite<&[u8]>copy pathBytesAPIIAI/Callgrind Measurements
Same temporary harness. Instruction counts:
Retained packet construction is effectively flat by IAI:
67,817 Iron main vs67,931 Irhere for the 32 KiB retained packet case.